Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ppaux out of existence. #58140

Merged
merged 70 commits into from
Mar 15, 2019
Merged

Refactor ppaux out of existence. #58140

merged 70 commits into from
Mar 15, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 4, 2019

A long-time coming, this PR reorganizes and rewrites the pretty-printing architecture of rustc, specifically the parts that involve the typesystem (which used to be in rustc::util::ppaux).

Note: these commits used to be in #57967 before being split off.

The new API (i.e. the Printer and PrettyPrint traits) is in rustc::ty::print.

Design points, roughly:

  • using associated types in Printer to allow building e.g. an AST, not just printing as a side-effect
  • several overloading points for implementers of PrettyPrinter, e.g. how <...> is printed
  • for fmt::Display impls, the value to print is lifted to the ty::tls tcx, and everything after that stays within the ty::print API, which requires 'tcx to match between values and the printer's tcx, without going through fmt::Display again

Most of the behavior is unchanged, except for a few details, which should be clear from the test changes.

r? @nikomatsakis

Fixes #55464

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2019
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am generally positive on this PR. I think it's a great step towards a better pretty-printing system. However, I would like to see more comments! I left some review comments at specific places that I think could use some documentation to help convey the design a bit. I also left various other nits and questions.

}
}

impl fmt::Debug for ty::ClosureUpvar<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to wonder if -- for some of these -- we should just use derive(Debug)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but maybe we should address that separately

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I want to get rid of as many of these as possible but wanted to avoid making decisions about them while refactoring.

}
}

impl fmt::Debug for ty::RegionKind {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we did not use derive(Debug) here because of things like ReFree and ReVar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but we can probably do whatever, this shouldn't be that used anyway.


impl fmt::Debug for ty::TraitRef<'tcx> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// HACK(eddyb) this is used across the compiler to print
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we convert this into a FIXME (ie, open an issue and cite it here)? As you and I discussed, I think the default {:?} should dump all data, but then we should have some way to just print the rest. I don't think we need to address it in this PR.

Copy link
Member Author

@eddyb eddyb Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think fmt::Display should also be <T as Trait<U>>, and that you'd have a method to print just the Trait<U> part, something like .print_only_trait_path().

Will open an issue.

@@ -472,6 +752,13 @@ impl<'a, 'tcx> Lift<'tcx> for ty::InstanceDef<'a> {
}
}

BraceStructLiftImpl! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did your PR to use proc macros in rustc land? Maybe we should convert these things to proc macros or custom derives... (obviously not in this PR, just thinking out loud)

@@ -0,0 +1,318 @@
use hir::map::{DefPathData, DisambiguatedDefPathData};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there should be some doc-comments in this file =) Some questions I would like to see answered:

  • I don't really understand the role of these traits
  • When is it appropriate to create a new printer (I've seen a few in this PR, but I can't quite tell what for)
  • When would it make sense to (e.g.) add a new print_my_type method to Printer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I don't really understand the role of these traits

The PR description tries to explain it but it probably fails to do so fully.

I need a better name than Printer/print_* for the general case, it's effectively a serialization scheme where the primitives are paths and several typesystem entities, only PrettyPrinter is really about "printing".

There might not even any point to having Printer (we can take the little logic it has and make it available some other way), and we can move its interface into PrettyPrinter, which can actually have defaults for (almost) everything (i.e. you can get FmtPrinter, or something similar, very easily, and then you can just customize the parts you care about).

  • When is it appropriate to create a new printer (I've seen a few in this PR, but I can't quite tell what for)

It's not clear to me right now too, other than to replace the old ItemPathBuffer trait.

In #57967 you can see I created a Printer in rustc_codegen_utils::symbol_names::mw which relies on the associated types, but almost everything else is PrettyPrinter and that you would create whenever FmtPrinter is not enough.

I would even take the RegionHighlightMode out of FmtPrinter and move it to wherever most of the lifetime placeholder code lives, but I felt that was even more churn to add to this PR.

  • When would it make sense to (e.g.) add a new print_my_type method to Printer?

When you want to overload it in an implementation, without having to reimplement printing for whatever happens to contain it (e.g. paths, types, etc.). This is usually the case for all "typesystem entities".

For example, @varkor will have to add a print_const method that takes a ty::LazyConst<'tcx> (unless @oli-obk wants to rename it again), because printers will want to choose how they print constants without having to handle that in a few separate places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing up these notes in a comment would be great

}
}

// HACK(eddyb) this duplicates `FmtPrinter`'s `path_generic_args`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look too closely here, but is it just not convenient to write a helper fn or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I did that in https://github.com/alexcrichton/rustc-demangle/pull/23/files#diff-8519e21583289947083908b68780a70cR915 but that still duplicates a bit of logic.

I would need to get rid of Printer, I guess? PrettyPrinter can tolerate an API that's more flexible in terms of mixing how/when things are printed.

};
}

// HACK(eddyb) this is separate because `ty::RegionKind` doesn't need lifting.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This comment kind of lacks context. Separate from what, for example? I guess you mean:

Implement Display for RegionKind separately. We can't use the macro below because RegionKind doesn't need lifting.

I don't personally feel a "HACK" tag is necessary here but feel free to re-insert if you like =)

&'tcx ty::List<ty::ExistentialPredicate<'tcx>>,

// HACK(eddyb) these are exhaustive instead of generic,
// because `for<'gcx: 'tcx, 'tcx>` isn't possible yet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good comment =)

vid: Option<ty::Region<'tcx>>,
expected_has_vid: Option<usize>,
actual_has_vid: Option<usize>,
any_self_ty_has_vid: bool,
) {
// HACK(eddyb) maybe move this in a more central location.
#[derive(Copy, Clone)]
struct Highlighted<'a, 'gcx, 'tcx, T> {
Copy link
Contributor

@nikomatsakis nikomatsakis Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem a bit odd for this struct to be private to this function, when the region highlighting machinery itself all likes in the generic ty::print module. We could move it there I guess, but I'm also ok to hold off a bit, because of the reasons in this comment -- basically this mechanism is perhaps not as general as I was initially thinking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would want to move it out of ty::print::pretty, but maybe not in this PR.

Error = fmt::Error,
>,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about this impl doesn't feel right to me. I mean, I'm not saying it's buggy or anything like that. I guess it's fine. I think I'm just a bit sad about how this Display impl creates the pretty-printer, rather than adding a bit of configuration to an existing one...somehow...

Basically, I was sort of hoping we can get to a place where you can kind of "compose" flags like this, so that you could write:

foo.with_highlight(...).with_highlight(...).with_fuzzy_bear(...)

and thus print foo with all those things. Or..something like that. Anyway, I don't think that's necessarily something to address here, it's just something I'd like to think about maybe in some future PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to take configuration out of FmtPrinter (and do it in types instead of data, basically).

The reason the printer is created here is that this is a printing entry-point.

That is, Highlighted<T> display-formats similar to T does, but with different configuration.

The T wouldn't have been ty::print::Print::print-ed without going through fmt::Display, so there's no meaningful "existing printer".

An earlier version of this code created a pretty-printer once but when someone touched the diagnostics I had to introduce this wrapper type mid-rebase.

@bors
Copy link
Contributor

bors commented Feb 7, 2019

☔ The latest upstream changes (presumably #58254) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

I don't really have enough energy to track this PR. So r=me once rebased. Ideally, we'd add a few more comments about the various things I highlighted in my review as well.

@michaelwoerister
Copy link
Member

Hey @eddyb, I talked to @nikomatsakis about this PR and he says that he think this PR is good to be merged if you add some more documentation. It would be great to get it merged soon so we can make progress on #57967.

@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2019
@eddyb eddyb force-pushed the advent-of-print branch 2 times, most recently from 5a5e211 to 9e3c439 Compare March 14, 2019 17:19
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:105fca08:start=1552584032094632030,finish=1552584034544468315,duration=2449836285
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:06:52]    Compiling rustc_macros v0.1.0 (/checkout/src/librustc_macros)
[00:07:41]    Compiling syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:13:59]    Compiling rustc_typeck v0.0.0 (/checkout/src/librustc_typeck)
[00:13:59]    Compiling rustc_mir v0.0.0 (/checkout/src/librustc_mir)
[00:14:02] error: unused import: `DefIdTree`
[00:14:02]   --> src/librustc_mir/borrow_check/mod.rs:15:23
[00:14:02]    |
[00:14:02] 15 | use rustc::ty::{self, DefIdTree, TyCtxt};
[00:14:02]    |
[00:14:02]    = note: `-D unused-imports` implied by `-D warnings`
[00:14:02] 
[00:14:14] error: aborting due to previous error
---
156800 ./obj/build/bootstrap/debug/incremental
156416 ./src/llvm-project/clang
150472 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc
141936 ./obj/build/bootstrap/debug/incremental/bootstrap-5f86g9tk67ex
141932 ./obj/build/bootstrap/debug/incremental/bootstrap-5f86g9tk67ex/s-facfbzx4za-101gpta-3clbg54leqh9a
108532 ./src/llvm-project/lldb
97584 ./src/llvm-project/clang/test
95632 ./.git
93100 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@eddyb
Copy link
Member Author

eddyb commented Mar 14, 2019

@nikomatsakis @michaelwoerister I've opened #59189 to track that this is not in the best of shapes, in order to get it merged ASAP, it's been bitrotting faster than I can rebase it so far.

@bors r=nikomatsakis p=777

@bors
Copy link
Contributor

bors commented Mar 14, 2019

📌 Commit fdf8d1b has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2019
@bors
Copy link
Contributor

bors commented Mar 14, 2019

⌛ Testing commit fdf8d1b with merge 71d6e76...

bors added a commit that referenced this pull request Mar 14, 2019
Refactor ppaux out of existence.

A long-time coming, this PR reorganizes and rewrites the pretty-printing architecture of rustc, specifically the parts that involve the typesystem (which used to be in `rustc::util::ppaux`).

*Note: these commits used to be in #57967 before being split off.*

The new API (i.e. the `Printer` and `PrettyPrint` traits) is in `rustc::ty::print`.

Design points, roughly:
* using associated types in `Printer` to allow building e.g. an AST, not just printing as a side-effect
* several overloading points for implementers of `PrettyPrinter`, e.g. how `<...>` is printed
* for `fmt::Display` impls, the value to print is lifted to the `ty::tls` `tcx`, and everything after that stays within the `ty::print` API, which requires `'tcx` to match between values and the printer's `tcx`, without going through `fmt::Display` again

Most of the behavior is unchanged, except for a few details, which should be clear from the test changes.

r? @nikomatsakis

Fixes #55464
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 14, 2019
@@ -14,7 +14,7 @@ LL | let x: () = <i8 as Foo<'static, 'static, u32>>::bar::<'static, char>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found fn item
|
= note: expected type `()`
found type `fn() {<i8 as Foo<ReStatic, ReStatic, u32>>::bar::<ReStatic, char>}`
found type `fn() {<i8 as Foo<ReStatic, ReStatic>>::bar::<ReStatic, char>}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a regression in the output?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Zverbose no longer control whether defaulted parameters are printed. This is because keeping the old behavior would mean no/less shared infrastracture for determining which generic arguments are used for which path segment.

Also, there is no information lost, because we don't check for an "approximation" of the default, but a perfectly exact match.
SO you can always refer to the definition (of Foo, in this case) for the default.

@@ -462,11 +462,11 @@ note: required by `check`
LL | fn check<T: Impossible>(_: T) {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `fn() -> namespace_mix::c::E {namespace_mix::c::E::TV}: Impossible` is not satisfied
error[E0277]: the trait bound `fn() -> namespace_mix::c::E {namespace_mix::xm7::TV}: Impossible` is not satisfied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this is a reintroduction of #56943.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the tests for #56943 are changed by this PR.

AFAICT, this is actually a bug fix! Ignoring irrelevant items:

pub mod c {
    pub enum E {
        V {},
        TV(),
        UV,
    }
}
pub mod xm7 {
    pub use ::c::E::*;
}

xm7::TV and c::E::TV are both public, and the former path is shorter, which is how we choose which form to print, here's a quick example:

2 |     let () = std::collections::hash_map::HashMap::new();
  |         ^^ expected struct `std::collections::HashMap`, found ()

Just to make sure, I also tested with mod E instead of enum E, i.e.:

pub mod c {
    pub mod E {
        pub fn TV() {}
    }
}
pub mod xm7 {
    pub use ::c::E::*;
}
3 |     let () = a::c::E::TV;
  |         ^^ expected fn item, found ()
  |
  = note: expected type `fn() {a::xm7::TV}`

As you can see, Rust nightly already prints xm7::TV in that case (note that this needs to be in a different crate, otherwise the full crate-local path will be printed anyway).

enum variants weren't properly handled before the "rustc: slice substs in ty::print instead of passing the full ones." commit, and they're only partially handled afterwards:
That is, you only get the behavior that considers reexports of variants if it has no generic parameters (because it needs to show you Enum::<...>::Variant, there's no special-casing to allow Variant::<...>).
(I also tried this out to confirm my intuition)

@eddyb
Copy link
Member Author

eddyb commented Mar 15, 2019

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 15, 2019

📌 Commit dbf19c3 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2019
bors added a commit that referenced this pull request Mar 15, 2019
Refactor ppaux out of existence.

A long-time coming, this PR reorganizes and rewrites the pretty-printing architecture of rustc, specifically the parts that involve the typesystem (which used to be in `rustc::util::ppaux`).

*Note: these commits used to be in #57967 before being split off.*

The new API (i.e. the `Printer` and `PrettyPrint` traits) is in `rustc::ty::print`.

Design points, roughly:
* using associated types in `Printer` to allow building e.g. an AST, not just printing as a side-effect
* several overloading points for implementers of `PrettyPrinter`, e.g. how `<...>` is printed
* for `fmt::Display` impls, the value to print is lifted to the `ty::tls` `tcx`, and everything after that stays within the `ty::print` API, which requires `'tcx` to match between values and the printer's `tcx`, without going through `fmt::Display` again

Most of the behavior is unchanged, except for a few details, which should be clear from the test changes.

r? @nikomatsakis

Fixes #55464
@bors
Copy link
Contributor

bors commented Mar 15, 2019

⌛ Testing commit dbf19c3 with merge ad8a3eb...

@bors
Copy link
Contributor

bors commented Mar 15, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: nikomatsakis
Pushing ad8a3eb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 15, 2019
@bors bors merged commit dbf19c3 into rust-lang:master Mar 15, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #58140!

Tested on commit ad8a3eb.
Direct link to PR: #58140

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-fail → build-fail (cc @nrc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Mar 15, 2019
Tested on commit rust-lang/rust@ad8a3eb.
Direct link to PR: <rust-lang/rust#58140>

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-fail → build-fail (cc @nrc @Xanewok, @rust-lang/infra).
@eddyb eddyb deleted the advent-of-print branch March 15, 2019 18:20
bors added a commit that referenced this pull request May 31, 2019
Introduce Rust symbol mangling scheme.

This is an implementation of a "feature-complete" Rust mangling scheme, in the vein of rust-lang/rfcs#2603 ~~- but with some differences, see rust-lang/rfcs#2603 (comment) for details~~ (@michaelwoerister integrated my proposed changes into the RFC itself).

On nightly, you can now control the mangling scheme with `-Z symbol-mangling-version`, which can be:
* `legacy`: the older mangling version, still the default currently
* `v0`: the new RFC mangling version, as implemented by this PR

To test the new mangling, set `RUSTFLAGS=-Zsymbol-mangling-version=v0` (or change [`rustflags` in `.cargo/config.toml`](https://doc.rust-lang.org/cargo/reference/config.html#configuration-keys)). Please note that only symbols from crates built with that flag will use the new mangling, and that tool support (e.g. debuggers) will be limited initially, and it may take a while for everything to be upstreamed. However, `RUST_BACKTRACE` should work out of the box with either mangling version.

<hr/>

The demangling implementation PR is rust-lang/rustc-demangle#23
~~(this PR already uses it via a git dependency, to allow testing)~~.

Discussion of the *design* of the mangling scheme should still happen on the RFC, but this PR's specific implementation details can be reviewed in parallel.

*Notes for reviewers*:
* ~~only the last 6 commits are specific to this branch, if necessary I can open a separate PR for everything else (it was meant to be its own small refactoring, but it got a bit out of hand)~~
~~based on #58140~~
* the "harness" commit is only there because it does some extra validation (comparing the demangling from `rustc-demangle` to the compiler's pretty-printing, adjusted slightly to produce the same output), that I would like to try on crater
* ~~there is the question of whether we should turn on the new mangling now, wait for tools to support it (I'm working on that), and/or have it under a `-Z` flag for now~~ (we're gating this on `-Z symbol-mangling-version=v0`, see above)

r? @nikomatsakis / @michaelwoerister cc @rust-lang/compiler
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 15, 2020
…acrum

Remove vestigial CI job msvc-aux.

This CI job isn't really doing anything, so it seems prudent to remove it.

For some history:
* This was introduced in rust-lang#48809 when the msvc job was split in two to keep it under 2 hours (oh the good old days). At the time, this check-aux job did a bunch of things:
    * tidy
    * src/test/pretty
    * src/test/run-pass/pretty
    * src/test/run-fail/pretty
    * src/test/run-pass-valgrind/pretty
    * src/test/run-pass-fulldeps/pretty
    * src/test/run-fail-fulldeps/pretty
* Tidy was removed in rust-lang#60777.
* run-pass and run-pass-fulldeps moved to UI in rust-lang#63029
* src/test/pretty removed in rust-lang#58140
* src/test/run-fail moved to UI in rust-lang#71185
* run-fail-fulldeps removed in rust-lang#51285

Over time through attrition, the job was left with one lonely thing: `src/test/run-pass-valgrind/pretty`. And of course, this wasn't actually running the "pretty" tests. The normal `run-pass-valgrind` tests ran, and then when it tried to run in "pretty" mode, all the tests were ignored because compiletest thought nothing had changed (apparently compiletest isn't fingerprinting the mode?  Needs more investigation…). `run-pass-valgrind` is already being run as part of `x86_64-msvc-1`, so there's no need to run it here.

I've taken the liberty of removing `src/test/run-pass-valgrind/pretty` as a distinct test. I'm guessing from the other PR's that the pretty tests should now live in `src/test/pretty`, and that the team has moved away from doing pretty tests on other parts of the `src/test` tree.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 15, 2020
…acrum

Remove vestigial CI job msvc-aux.

This CI job isn't really doing anything, so it seems prudent to remove it.

For some history:
* This was introduced in rust-lang#48809 when the msvc job was split in two to keep it under 2 hours (oh the good old days). At the time, this check-aux job did a bunch of things:
    * tidy
    * src/test/pretty
    * src/test/run-pass/pretty
    * src/test/run-fail/pretty
    * src/test/run-pass-valgrind/pretty
    * src/test/run-pass-fulldeps/pretty
    * src/test/run-fail-fulldeps/pretty
* Tidy was removed in rust-lang#60777.
* run-pass and run-pass-fulldeps moved to UI in rust-lang#63029
* src/test/pretty removed in rust-lang#58140
* src/test/run-fail moved to UI in rust-lang#71185
* run-fail-fulldeps removed in rust-lang#51285

Over time through attrition, the job was left with one lonely thing: `src/test/run-pass-valgrind/pretty`. And of course, this wasn't actually running the "pretty" tests. The normal `run-pass-valgrind` tests ran, and then when it tried to run in "pretty" mode, all the tests were ignored because compiletest thought nothing had changed (apparently compiletest isn't fingerprinting the mode?  Needs more investigation…). `run-pass-valgrind` is already being run as part of `x86_64-msvc-1`, so there's no need to run it here.

I've taken the liberty of removing `src/test/run-pass-valgrind/pretty` as a distinct test. I'm guessing from the other PR's that the pretty tests should now live in `src/test/pretty`, and that the team has moved away from doing pretty tests on other parts of the `src/test` tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants